Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DMS-26] Generate dependencies #193

Merged
merged 26 commits into from
Jul 23, 2024
Merged

[DMS-26] Generate dependencies #193

merged 26 commits into from
Jul 23, 2024

Conversation

simpat-adam
Copy link
Contributor

No description provided.

Copy link

github-actions bot commented Jul 16, 2024

Test Results

391 tests  +7   372 ✅ +7   2m 22s ⏱️ +12s
  5 suites ±0    19 💤 ±0 
  5 files   ±0     0 ❌ ±0 

Results for commit d4677db. ± Comparison against base commit d186294.

♻️ This comment has been updated with latest results.

if (resourceName == "EducationOrganization")
{
resourceName = "School";
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open to suggestions here. Definitely a code smell.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an idea. Read the abtractResources section to get EducationOrganization and GeneralStudentProgramAssociation. Then scan the resources for the first one that has isSubclass: true and matches each. Use that instead.

It might work, or it might be too simplistic in that it picks e.g. EducationOrganizationNetwork which isn't as "connected" (from a dependency graph perspective) as School. In which case you'd need to do some sort of "connectedness" analysis of subclasses to select the one to substitute.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another idea, ignore the abstract superclasses to start and get an ordering. Then find all the subclasses in the ordering and insert the superclass right after the last subclass. That's probably the easiest way.

@simpat-adam simpat-adam force-pushed the DMS-26 branch 6 times, most recently from e32144e to 7602b58 Compare July 17, 2024 13:53
@simpat-adam simpat-adam marked this pull request as ready for review July 17, 2024 14:00
@simpat-adam simpat-adam force-pushed the DMS-26 branch 2 times, most recently from 3cf5c75 to f673452 Compare July 17, 2024 19:03
Copy link
Contributor

@bradbanister bradbanister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see some unit testing of GetDependencies(). Provided suggestions on making testability easier.

src/core/EdFi.DataManagementService.Core/ApiService.cs Outdated Show resolved Hide resolved
src/core/EdFi.DataManagementService.Core/ApiService.cs Outdated Show resolved Hide resolved
src/core/EdFi.DataManagementService.Core/ApiService.cs Outdated Show resolved Hide resolved
@simpat-adam simpat-adam force-pushed the DMS-26 branch 2 times, most recently from 867d22b to 3f5ddbb Compare July 19, 2024 16:46
@simpat-adam simpat-adam force-pushed the DMS-26 branch 2 times, most recently from 902c366 to 0ec5ea5 Compare July 22, 2024 12:35
Copy link
Contributor

@bradbanister bradbanister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Very minor naming recommendation.

}
""";

private readonly string _expectedDescriptor =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_expectedDependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha! Oops.

}

[TestFixture]
public class Given_A_Dependency_Calculator() : DependencyCalculatorTests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍👍

}
""";

private readonly string _expectedDescriptor =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_expectedDependencies?

@bradbanister bradbanister merged commit 867ec20 into main Jul 23, 2024
12 checks passed
@bradbanister bradbanister deleted the DMS-26 branch July 23, 2024 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants